I am still a little surprised that nonzeros returns a copy. I can understand the reasoning behind it (the sparse object should not become corrupted). But is this a common pattern? This kind of cries for something like an read-only array.

This would certainly be the expected behaviour. For access to the underlying data structures of SparseMatrixCSC, we should probably have accessor methods. We will do that early in 0.4. For now, people already directly pick up S.nzval.

Returning a copy might be the expected behavior, but it also makes nonzeros essentially useless for the vast majority of cases that I'm aware of. The name nonzeros! seems like an abuse of notation; it doesn't make any modifications to its argument.

I don't really see any benefit that these methods bring. If you're using nonzeros!, you already know enough about the data structure to go ahead and use S.nzval. If you don't know the data structure, then the list of nonzero entries isn't too useful since you don't know what indices they correspond to, and findnz would be used instead. This plus the huge naming conflict leads me to say that we should just get rid of nonzeros altogether.

The ! should only be used if the argument is modified.

@mlubin I have used nonzeros in this manner quite a bit when I used to work with MATLAB. I think the name is not confusing, and the behaviour is not a surprise to those who expect it. nonzeros! is already gone.

Just out of curiosity, @eldadHaber - Can your use case of nonzeros be replaced by findnz? The only downside of findnz is that it would allocate memory for indexes, which you may not want.

Cc: @tkelman

@ViralBShah, what's a typical use case where one would want to make a copy of nzval?

find()/findnz() over sparse matrices no longer filters out stored zeros

Not sure that's a good idea, you're just moving the inconsistent treatment of zeros from nonzeros into find. I think find should be left alone, if a function for converting CSC to COO leaving stored zeros intact is desired I think it should be a separate function (or conversion if we add a full-fledged COO type).

A construction flag in sparse to allow explicit zeros sounds reasonable.

At this point I don't see a whole lot of use for nonzeros(A) relative to A.nzval, except I guess for the sake of naming familiarity from Matlab where you don't typically have direct access to the underlying CSC data.

A.nzval is not recommended. We really need to have an api for that, or else we will be stick with the internal structure.

I am with @mlubin and @tkelman here. At this point nonzeros is just a convenience function for matlab users and funnily the Matlab function should be even more efficient as it has COW semantics.

Of course we have similar issues with all the temporaries that are created in the arithmetic vector operations but unlike + where one really has no chance to do anything but create a temporary array, here the array is already allocated.

So from the user perspective "the right thing" to do is currently to use the fields of SparseMatrixCSC. This is ok if we decide that fields are part of an interface (see #4935 and #1974). But reading through both issues I don't see that there is already a conclusion about this. If we decide that only functions are the public interface and field overloading is not coming, then we would need accessor methods like e.g. nonzeros.

Maybe we should introduce a convention that functions that accessor functions that may corrupt the state of an object should be named nonzeros💀 ;-)

But joking aside I think that we need decisions on #1974 as this will affect the way Base will involve during the journey to 1.0

To this PR, I still like @mlubin 's (#5538) naming convention better as it is more explicite and circumvents the uncertainty that we have on structural and actual non-zeros. And I still do not get the pressing reason to make Matlab compatibility so important (other than 1 user beeing unhappy). But as we have talked about that already in #6769 I think it is time to just make a decision for 0.3.

What's the concern with field access, name stability? Wanting to apply the same code to other sparse formats with potentially different field names?

I doubt I'd use an API that's dramatically slower than field access when both are equally convenient. Having a non-copy field accessor function could be worth using, the concern about modification there is more or less the same as with slices, transposes, etc potentially returning views.

@tkelman. Yes its about name stability and "good practice". It is always good to separate the interface from the implementation. Using functions for interfaces is whats currently done in Base (e.g. size, length) and performance should not be an issue when using inlining. But again looking at #4935 and #1974 there is not yet a clear conclusion whether this is the route to follow in the future. Field overloading would break that paradigm and make fields part of an interface. Note that my comment is about a non-copying accessor.

It is true that one could argue that the interface of SparseMatrixCSC is nothing that will be reused so that information hiding is probably not that important here. Still I think it would be good to have conventions that we follow even in cases where it is probably not so important.

I understanding some of the reasoning behind the copy. However, knowing that nzval will be copied in nonzeros would make me circumvent this function altogether and go directly to nzval in practice.

I would suggest nonzeros returns the vector of the non zero values itself (or a view thereof), so that they can be accessed without being copied or directly manipulated. It should also be clearly documented and we should warn the user about potential consequence of modifying the elements there.

One choice is to leave nonzeros as is and have a separate api that returns all the internal vectors without making copies.

I agree with @lindahua; nonzeros should just return the data, not a copy.

nonzeros no longer returns a copy.

Cool, now on to the next controversial issue :)
I'd agree with @tkelman that find and findnz should remove the explicitly stored zeros. If a user is interacting with a sparse matrix at this higher level, I don't think the concept of structural nonzeros needs to be introduced (except maybe as a keyword argument?)

I wasn't sure about that one, but wanted to try it out. I think it is best to remove it and avoid the cognitive overload.

Alright, what else do you want? :-)

Good point - done.

I'm satisfied on all of the substantial issues here. I think we're just missing deprecations from the 0.2 version of nnz that applied to general vectors:

@deprecate nnz(v::AbstractVector) countnz

(assuming that works correctly). Also NEWS has a mention of nfilled that should be updated.

Why only AbstractVector? What about AbstractArray?

I mean we should do it for StridedArray.

That's probably right.

Anyone have remaining strong objections?

LGTM. Thanks @ViralBShah for taking all objections into account!

@ViralBShah I think it is great. Thanks.